Revert sum literal integer change #13961

hauntsaninja · 2022-10-29T18:37:56Z

This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing to undo. Patching this in typeshed also feels weird, since there's a more general soundness issue. If a typevar has a bound or constraint, we might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)

This is allegedly causing large performance problems, see 13821 typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing to undo. Patching this in typeshed also feels weird. If a typevar has a bound or constraint, we might not want to solve it to a Literal. If we can confirm the performance regression or fix the unsoundness within mypy, I might pursue upstreaming this in typeshed.

hauntsaninja · 2022-10-29T18:38:10Z

cc @AlexWaygood , in case you have opinions

AlexWaygood · 2022-10-29T18:41:07Z

cc @AlexWaygood , in case you have opinions

my opinion is that huge performance regressions are bad, feel free to go ahead 👍 :)

hauntsaninja · 2022-10-29T18:43:25Z

Lol, alleged performance regressions though :-) I meant more on the upstreaming the revert question

hauntsaninja · 2022-10-29T18:59:26Z

I also filed microsoft/pyright#4108 , so we'll see if Eric has a point of view

github-actions · 2022-10-29T19:03:00Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

This is allegedly causing large performance problems, see 13821 typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing to undo. Patching this in typeshed also feels weird, since there's a more general soundness issue. If a typevar has a bound or constraint, we might not want to solve it to a Literal. If we can confirm the performance regression or fix the unsoundness within mypy, I might pursue upstreaming this in typeshed. (Reminder: add this to the sync_typeshed script once merged)

Makes sure we continue to cherry pick #13961

AlexWaygood · 2022-10-29T21:27:05Z

I meant more on the upstreaming the revert question

Hah, I thought this was a PR to the typeshed repo when I posted my comment :)

The stub for sum has got quite a lot more complicated than it used to be over the course of many PRs during the last ~6 months or so. Some of those PRs were by me, some were by other people. In the end it seems like it's a surprisingly difficult function to write an accurate stub for, so maybe we'd be better off now if we hadn't had any of those sum PRs. There's still a few open typeshed issues about sum that I don't really know how to fix, and that have been caused by some of the recent increases in complexity.

I filed the PR adding _LiteralInteger to one of the sum overloads in response to a user complaint, so if we revert that PR over at typeshed, we should probably reopen that issue as once-again-unsolved.

If the sum change did cause a massive performance regression, I'd be curious to know whether pyright has Literal algorithms with the same bad time complexity, and whether it's therefore facing similar performance issues on codebases that have heavy use of sum. A performance regression on that scale is obviously completely unacceptable, so if it's happening to all type checkers, it definitely needs to be reverted over at typeshed.

hauntsaninja · 2022-10-29T23:48:01Z

Yeah, I guess in my mind there are two slightly different things at play here:

The (alleged) performance issue. I'm assuming this is mypy specific. No one's provided a good minimal repro and I haven't had time to investigate, so there's actually not too much to go on. Hence my default would be to not make a change to typeshed for a mypy perf issue (unless e.g. some other typeshed demographic has a problem with it).
The underlying soundness issue. This could conceivably be a thing fixed in mypy and pyright (although Eric closed as a won't fix for now). Although as I'm writing this, I realise that the solution I had in mind wouldn't work.

I'm currently thinking if I were to make a change to typeshed it might be pragmatic to change Iterable[bool | _LiteralInteger] to Iterable[bool | Literal[0, 1]]. This has the following advantages:

Still fixes the original typeshed report / the way this most likely comes up in real world code
Removes a weird soundness cliff at 26
I anticipate that if we are able to repro the perf issue, that reducing the size of the union would fix. There are some places where large unions make mypy slow

AlexWaygood · 2022-10-30T15:03:40Z

I'm currently thinking if I were to make a change to typeshed it might be pragmatic to change Iterable[bool | _LiteralInteger] to Iterable[bool | Literal[0, 1]].

I'd be fine with that. The only reason I went with _LiteralInteger was because we already had the _PositiveInteger and _NegativeInteger aliases in builtins.pyi (so it was easy to do), and (forgetting that mypy has performance issues with large Literals), it seemed good to make the Literal as large as possible in order to avoid false positives.

Removes a weird soundness cliff at 26

Though there's a certain symmetry here with pow(), which has an awkward "oops, now we infer everything as Any" cliff at 26 :))

AlexWaygood · 2022-10-30T15:09:18Z

I agree that the underlying soundness issue is a thing, and is a pain. I think it's come up a few other times as well (but Eric's also right that it's not a very common issue). No idea how we could fix it, though — we'd probably need to somehow formalise exactly when it's unsound to solve a TypeVar with a Literal. Eric's probably right that this is maybe best discussed over at python/typing.

This is allegedly causing large performance problems, see 13821 typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing to undo. Patching this in typeshed also feels weird, since there's a more general soundness issue. If a typevar has a bound or constraint, we might not want to solve it to a Literal. If we can confirm the performance regression or fix the unsoundness within mypy, I might pursue upstreaming this in typeshed. (Reminder: add this to the sync_typeshed script once merged)

hauntsaninja mentioned this pull request Oct 29, 2022

mypy 0.982 run 15 minutes (previous version needs only one minute) #13821

Closed

hauntsaninja merged commit 5319fa3 into python:master Oct 29, 2022

hauntsaninja deleted the revert-sum branch October 29, 2022 19:47

hauntsaninja mentioned this pull request Oct 29, 2022

Revert sum literal changes on an ongoing basis #13962

Merged

hauntsaninja added a commit that referenced this pull request Oct 29, 2022

Revert sum literal changes on an ongoing basis (#13962)

40d04a5

Makes sure we continue to cherry pick #13961

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert sum literal integer change #13961

Revert sum literal integer change #13961

hauntsaninja commented Oct 29, 2022 •

edited

Loading

hauntsaninja commented Oct 29, 2022

AlexWaygood commented Oct 29, 2022

hauntsaninja commented Oct 29, 2022 •

edited

Loading

hauntsaninja commented Oct 29, 2022 •

edited

Loading

github-actions bot commented Oct 29, 2022

AlexWaygood commented Oct 29, 2022

hauntsaninja commented Oct 29, 2022 •

edited

Loading

AlexWaygood commented Oct 30, 2022

AlexWaygood commented Oct 30, 2022 •

edited

Loading

Revert sum literal integer change #13961

Revert sum literal integer change #13961

Conversation

hauntsaninja commented Oct 29, 2022 • edited Loading

hauntsaninja commented Oct 29, 2022

AlexWaygood commented Oct 29, 2022

hauntsaninja commented Oct 29, 2022 • edited Loading

hauntsaninja commented Oct 29, 2022 • edited Loading

github-actions bot commented Oct 29, 2022

AlexWaygood commented Oct 29, 2022

hauntsaninja commented Oct 29, 2022 • edited Loading

AlexWaygood commented Oct 30, 2022

AlexWaygood commented Oct 30, 2022 • edited Loading

hauntsaninja commented Oct 29, 2022 •

edited

Loading

hauntsaninja commented Oct 29, 2022 •

edited

Loading

hauntsaninja commented Oct 29, 2022 •

edited

Loading

hauntsaninja commented Oct 29, 2022 •

edited

Loading

AlexWaygood commented Oct 30, 2022 •

edited

Loading